AWS: fix Iceberg to Glue schema conversion#3887
Conversation
jackye1995
left a comment
jackye1995
left a comment
There was a problem hiding this comment.
Thanks, we definitely missed a lot of edge cases here. Could you add more tests, and also add integration tests for this? If you don't have an AWS environment to run, I can run it after you publish.
| return ImmutableMap.of(ICEBERG_FIELD_USAGE, fieldUsage, | ||
| ICEBERG_FIELD_TYPE_TYPE_ID, field.type().typeId().toString(), | ||
| ICEBERG_FIELD_TYPE_STRING, toTypeString(field.type()), | ||
| ICEBERG_FIELD_TYPE_TYPE_ID, safeString(field.type().typeId().toString()), |
There was a problem hiding this comment.
this cannot avoid a null because you are already calling toString()
| import org.slf4j.LoggerFactory; | ||
| import software.amazon.awssdk.services.glue.model.Column; | ||
| import software.amazon.awssdk.services.glue.model.DatabaseInput; | ||
| import software.amazon.awssdk.services.glue.model.Schedule; |
|
Discussed with Kanou offline, it seems like the current way of exposing schema is too complex, and we will start simple and remove the partition spec part of it, and also remove some unnecessary annotations. Kanou will provide the update soon. |
| } | ||
|
|
||
| private static void addColumnWithDedupe(List<Column> columns, Set<String> dedupe, | ||
| NestedField field, String fieldName) { |
There was a problem hiding this comment.
the fieldName parameter is not necessary, it's always field.name()
| return ImmutableMap.of(ICEBERG_FIELD_USAGE, fieldUsage, | ||
| ICEBERG_FIELD_TYPE_TYPE_ID, field.type().typeId().toString(), | ||
| ICEBERG_FIELD_TYPE_STRING, toTypeString(field.type()), | ||
| private static Map<String, String> convertToParameters(NestedField field) { |
There was a problem hiding this comment.
this method can be combined to addColumnWithDedupe for simplicity
|
thanks, mostly look good to me, I will verify the aws integration test while CI is running. |
|
confirmed AWS integ test successful. Waiting for CI |
jackye1995
left a comment
jackye1995
left a comment
There was a problem hiding this comment.
thanks for the work and quick fix, and congrats to the first PR!
2 participants
some bugs fixed:
y.zinstead ofz@jackye1995 @yyanyy